@jetm apart from any code comments people might have, I'd like a better description on the commit message.

Something along the lines of:
"EFI is becoming more common on embedded boards with the embracing of SystemReady-IR.
U-Boot which is most commonly used, is usually storing the EFI variables in a file in the ESP. That makes it impossible to support SetVariable at Runtime reliably, since the OS doesn't know how to access, read or write that file.

OS'es usually need SetVariable at runtime for three reasons

• Set the BootOrder
• Enable UEFI Secure Boot
• OSIndication to signal capsule updates on-disk.

Since the variables are stored in a file U-Boot enables SetVariable at runtime in the EFI config table and stores any updates in RAM. At the same file it creates 2 volatile variables

• RTStorageVolatile is the location of the file relative to the ESP
• VarTofile contains a binary dump of the EFI variables that need to be preserved on the file (BS, RT, NV)

U-Boot fills in the VarToFile dynamically on reads and that includes any updates the OS did in the meantime.

So, let's update efivar to do the same thing. Once a variable is written to the efivarfs, make sure efivars is mounted as rw and scan for the file "RTStorageVolatile". If we find that, copy the "VarToFile" contents in a file and preserve the variables across reboots"

also it's 'apalos', please fix the mention, some pool soul is getting notifications for EFI :)

@apalos Updated the commit message and PR description. Thank you!

We are interested in this change for Asahi Linux, since we are one such U-Boot platform with EFI vars only storable in the ESP. However, there is one additional quirk this code does not handle. Our systems can have multiple ESPs, since our platform is not UEFI-native: the platform has its own lower level concept of multiple OS installs that our UEFI implementation nests under, so a UEFI "container" with its own ESP is created for every OS install.

If I'm reading the code right here, this would clobber the EFI variables of every mounted ESP. That would work in 95% of cases (since normally only the current OS ESP is mounted) and cause all kinds of hard to debug trouble in the 5% of cases where someone with multiple OS installs ends up with other ESPs mounted.

Possible solutions:

1. Actually implement our bespoke protocol for identifying the right ESP, which is to read /proc/device-tree/chosen/asahi,efi-system-partition which will return the NUL-terminated GPT PARTUUID (in ASCII), and then only look for that one specific ESP.
2. Only allow ESPs mounted somewhere under /boot (if someone mounts another ESP it's almost certainly going to be under /mnt or /run/media or something)
3. Only target the first discovered ESP in /proc/mounts order (the real ESP is almost certainly going to be mounted first)
4. Standardize a mechanism for U-Boot to indicate the ESP PARTUUID (maybe RTStorageVolatile should also include the PARTUUID? This seems like the cleanest solution and very UEFI-like, though changing the format of that variable now would mean a breaking change vs. the state of things on the u-boot side as things stand today).

Of these 2 and 3 are obviously heuristics but avoid platform-specific code, while 1 is the "correct" solution but obviously very specific to our platform, and 4 makes the most sense to me TBH but would require a rethink of RTStorageVolatile.

Worth noting that this is not just about weird multi-ESP platforms like ours. If I'm reading the code correctly, this will also clobber the vars in any ESPs in external storage and things like that. That's going to cause some really hard to debug badness when people do stuff like plug in bootable USB sticks or SD cards from embedded systems into other systems using the same mechanism. I suspect this is a bad enough oversight to warrant changing the definition of RTStorageVolatile before it's too late and we're stuck with ambiguous ESP detection.

Hey Hector thanks for taking the time!

We are interested in this change for Asahi Linux, since we are one such U-Boot platform with EFI vars only storable in the ESP. However, there is one additional quirk this code does not handle. Our systems can have multiple ESPs, since our platform is not UEFI-native: the platform has its own lower level concept of multiple OS installs that our UEFI implementation nests under, so a UEFI "container" with its own ESP is created for every OS install.

If I'm reading the code right here, this would clobber the EFI variables of every mounted ESP. That would work in 95% of cases (since normally only the current OS ESP is mounted) and cause all kinds of hard to debug trouble in the 5% of cases where someone with multiple OS installs ends up with other ESPs mounted.

Yes it would, we also discussed this on the U-Boot ML while we reviewed the patches. I am happy to send an update once we figure this out.

One additional problem to keep in mind (which works with the current code) is OS installers. Some installers, e.g Ubuntu, contain an ESP in the installer image. They also create a new one during the installation. Storing the file in the pre-existing ESP won't work and the changes will be lost. We want the userspace tools to update the variables in the newly created partition.

Possible solutions:

1. Actually implement our bespoke protocol for identifying the right ESP, which is to read `/proc/device-tree/chosen/asahi,efi-system-partition` which will return the NUL-terminated GPT PARTUUID (in ASCII), and then only look for that one specific ESP.

2. Only allow ESPs mounted somewhere under /boot (if someone mounts another ESP it's almost certainly going to be under `/mnt` or `/run/media` or something)

3. Only target the _first_ discovered ESP in `/proc/mounts` order (the real ESP is almost certainly going to be mounted first)

4. Standardize a mechanism for U-Boot to indicate the ESP PARTUUID (maybe `RTStorageVolatile` should also include the PARTUUID? This seems like the cleanest solution and very UEFI-like, though changing the format of that variable now would mean a breaking change vs. the state of things on the u-boot side as things stand today).

The feature in U-Boot just got merged a few weeks ago, and I did not enable by default for a reason. We don't expect to break anything, so we are free to change this.

When I sent the patches, we had a short discussion about this and thought about storing the device path in RTStorageVolatile, but as I mentioned in that email, I think this delegates the problem but doesn't solve it....

The real problem is how U-Boot will decide/guess which ESP the OS is going to mount. If we can answer that we can encode whatever information we want in the variable to hint the OS. Even that though doesn't solve the OS installer problem I mentioned in the beginning...

Of these 2 and 3 are obviously heuristics but avoid platform-specific code, while 1 is the "correct" solution but obviously very specific to our platform, and 4 makes the most sense to me TBH but would require a rethink of RTStorageVolatile.

Worth noting that this is not just about weird multi-ESP platforms like ours. If I'm reading the code correctly, this will also clobber the vars in any ESPs in external storage and things like that. That's going to cause some really hard to debug badness when people do stuff like plug in bootable USB sticks or SD cards from embedded systems into other systems using the same mechanism. I suspect this is a bad enough oversight to warrant changing the definition of RTStorageVolatile before it's too late and we're stuck with ambiguous ESP detection.

It's not an oversight, we were very well aware of the problem -- even since my first RFC , but since we didn't have any valid use cases, we decided to keep the code that solves the 'installer problem'.

U-Boot itself is a bit problematic when it comes to the file and ESP variables. The SetVariable implementation will use the first file it finds on any ESP partition -- we don't have a way to select which file we want to write.

Out of the 4 solutions you proposed:

1. This will still suffer from the 'installer problem'
2. This might feel a bit hacky -- simply because the EFI allows multiple ESPs, but I also think it solves the majority of our problems
3. I prefer 2 over this
4. I can't find a reasonable way for u-boot to decide which ESP the OS is going to mount, If we can agree on this with distros, this is indeed the cleanest solution

I strongly prefer solution 2 here. I think that will also mean that we don't have to use blkid_probe_all(), which isn't the nicest thing to run on some machines.

I strongly prefer solution 2 here. I think that will also mean that we don't have to use blkid_probe_all(), which isn't the nicest thing to run on some machines.

Thanks @vathpela this helps, we all seem to prefer 2. The last bit I don't like on this PR, is that the code only works for 'efivar'. We ideally need it as part of the library so it works with efibootmgr out of the box.

We ideally need it as part of the library so it works with efibootmgr out of the box.

If you plumb it through as a full third method like vars vs efivarfs that should just happen.

Addressed @marcan @apalos comments.

• It searches for ESP file in three possible ESP partitions. I know in the PR discussion was only mentioned /boot, but /boot/efi and /efi are possible cases too, like systemd does. The code is simpler now because it's not using blkid anymore.
• Expose the required APIs in libefivar to be used by efibootmgr tool, which is in another repo. This will avoid duplicating code and it will not be required to change efibootmgr

@jetm some observations from testing

1. If you rename the ESP file and try to store a variable the app correctly figures out there's no file to store, but the variable in memory is updated. The file existence check must run before updating the in-memory copy
2. efivar: RSTStorageVolatile found with ubootefi.var
   efivar: VarToFile content saved at /boot/efi/ubootefi.var
   Don't display this on success, it doesn't mean much to users. Instead display an error only if RSTStorageVolatile points to a file that doesn't exist
3. Even if the app prints

efivar: RSTStorageVolatile found with ubootefi.var
efivar: VarToFile content saved at /boot/efi/ubootefi.var

The variables don't persist a reboot

@vathpela does the function moving around make sense to expose the functionality to the library? Or is there a bettwe way to do it?